Conditionally compile contracts instead of deciding at run-time #145229
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Hunh? All I can see in that PR is contract checks changing the optimized MIR of functions which is expected and should be fine. Disabled contract check MIR should be cleaned out as part of lowering to LLVM, or it should be immediately cleaned up by LLVM because it is under the equivalent of an |
How does one evaluate whether disabled contract check code is indeed cleaned up? You mentioned in #136578 (comment):
Could you elaborate on this please? Indeed one of the codegen tests (tests/codegen/cross-crate-inlining/leaf-inlining.rs) fails on that branch, and I don't have the expertise to judge what the consequences of that test failure are. I feel that contracts should not interfere with optimisations of the code in any way, since to me they serve as an executable piece of documentation, comparable to rustdoc tests. Having contracts get in the way of normal code will likely slow down their adoption. |
The initial implementation of contracts, despite requiring a compiler flag to enable runtime checking of contracts, still compiled contracts into function definitions, even when the compiler flag was disabled. This meant that contracts could not be safely added to functions without breaking optimisations, or even without potentially changing the behaviour of the function. This change guards macro expansion of the built-in contract macros with the contract-checks compiler flag. Additionally, it removes the contract_checks compiler intrinsic that was used to determine whether contract checks should be executed at runtime. Now, when contracts checks are compiled into the body of a function, they will always be executed.
dawidl022
force-pushed
the
contracts/conditional-macros-rebased
branch
from
c6064ad to
8600932
Compare
|
I haven't been involved in the contracts work, so it might make sense to reassign this to someone who is more involved in that effort. I would expect that long term, we definitely want contracts to be type checked even if we don't actually enforce them at runtime, is that correct? I would also expect that |
|
r? @celinval
Yes, I believe that is the case.
Agreed, it sounds desirable. I'm less sure however about relying on optimisations, when we can simply not compile the contracts when disabled. E.g. I'm not sure what the compile-time penalty might be of optimising out contracts compared to excluding them from compilation in the first place. |
Excluding them from compilation must only happen post MIR borrowck/after analysis as it shouldn't be user-facing, should it not? At this point removing them has to be a MIR optimization pass, so it can just be a more general "remove |
The initial implementation of contracts, despite requiring a compiler flag to enable runtime checking of contracts, still compiled contracts into function definitions, even when the compiler flag was disabled. This meant that contracts could not be safely added to functions without breaking optimisations, or even without potentially changing the behaviour of the function. This was blocking the addition of contracts to standard library functions in #136578.
This change guards macro expansion of the built-in contract macros with the
contract-checkscompiler flag. Additionally, it removes thecontract_checkscompiler intrinsic that was used to determine whether contract checks should be executed at runtime. Now, when contracts checks are compiled into the body of a function, they will always be executed.The change is motivated by the following discussion: #144438 (comment)
Contracts tracking issue: #128044
Known limitations:
contract-checksare disabled, contracts will not be parsed or type checked, meaning that they are susceptible to becoming out of sync with the rest of the codebase.